OLS-3632: Add oc-ols CLI scaffolding and kubeconfig integration - #1936
OLS-3632: Add oc-ols CLI scaffolding and kubeconfig integration#1936xiormeesh wants to merge 5 commits into
Conversation
|
@xiormeesh: This pull request references OLS-3632 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds the ChangesCLI foundation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/kubeconfig_test.go`:
- Around line 17-176: Convert the tests in cli/kubeconfig_test.go, including
writeTestKubeconfig and all TestLoadKubeConfig_* cases, from testing.T
assertions to the repository’s established Ginkgo/Gomega BDD structure and
matchers. Preserve each test’s existing coverage and expectations, then run make
test to validate the conversion.
In `@cli/kubeconfig.go`:
- Line 34: Update the error-wrapping paths in the kubeconfig loading flow to
define or reuse appropriate error constants and use each constant as the
fmt.Errorf prefix in the “%s: %w” format. Replace the literal prefixes at the
error sites corresponding to lines 34, 44, 51, and 100 while preserving the
existing wrapped errors and behavior.
- Around line 49-53: Trim whitespace from the contents read in the
BearerTokenFile branch before assigning the result to token, matching client-go
behavior. Add or update the token-file test to write the token followed by a
newline and verify the configured token excludes that newline.
- Around line 64-66: Update the tlsConfig initialization in the kubeconfig TLS
setup to assign ServerName from restConfig.TLSClientConfig.ServerName,
preserving kubeconfig tls-server-name overrides for SNI and certificate
validation. Add a regression test covering propagation of this override.
In `@cli/root.go`:
- Around line 13-26: Update NewRootCmd when constructing the root Cobra command
to bind its input, output, and error streams via SetIn, SetOut, and SetErr using
the injected IOStreams, ensuring help and error output use those streams instead
of process-level defaults.
In `@cli/version_test.go`:
- Around line 8-34: Convert the tests in cli/version_test.go (lines 8-34) and
cli/root_test.go (lines 9-60) from testing.T to the required Ginkgo Describe/It
structure with Gomega assertions, reusing an existing CLI suite or adding one
suite entrypoint as needed. Preserve the current version and root-command
behavior checks, including injected Version validation, and verify the changes
with make test.
In `@cli/version.go`:
- Around line 19-20: Wrap both command output write failures with dedicated
error constants: update cli/version.go lines 19-20 around the version-output
write to return fmt.Errorf with its new constant and the original error, and
update cli/root.go lines 21-22 around the default-mode error-output write
similarly with a separate constant; preserve successful output behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb0ae2a4-94e2-45d2-83a1-187a08028ddb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (11)
.ai/spec/how/cli.mdAGENTS.mdcli/kubeconfig.gocli/kubeconfig_test.gocli/root.gocli/root_test.gocli/testutil_test.gocli/version.gocli/version_test.gocmd/oc-ols/main.gogo.mod
Adds the foundation for the oc-ols kubectl/oc plugin: entry point, root command with global flags and default-mode dispatching, version command, and kubeconfig integration for bearer token extraction and TLS configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Define error constants for all error wrapping paths - Trim whitespace from bearer tokens (matches client-go behavior) - Preserve kubeconfig tls-server-name (ServerName) in TLS config - Bind Cobra streams to IOStreams via SetIn/SetOut/SetErr - Wrap fmt.Fprintf errors with ErrWriteOutput constant - Add ServerName propagation test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follow the repo's BDD test convention per CONTRIBUTING.md. Add suite_test.go entrypoint, convert all test files to Describe/It blocks with Gomega matchers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b546e81 to
912ac00
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cli/kubeconfig.go (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
AGENTS.mdaligned with this new CLI boundary.If the existing
AGENTS.mdupdate does not already cover this package, document kubeconfig defaults, token-only authentication, TLS/CA precedence, and the required test workflow.As per coding guidelines: “When architectural, structural, or conventional changes are made, suggest corresponding
AGENTS.mdedits.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/kubeconfig.go` around lines 31 - 34, Update the applicable AGENTS.md guidance for the LoadKubeConfig CLI boundary if these rules are not already documented: specify kubeconfig defaults, token-only authentication, TLS versus CA certificate precedence, and the required test workflow. Keep the guidance scoped to this package’s conventions and current behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/kubeconfig_test.go`:
- Around line 46-64: Update the negative-path tests using testKubeconfigNoToken
and the nonexistent-file case to assert the specific expected errors:
ErrNoBearerToken for the missing-token scenario and ErrLoadKubeConfig for the
missing-file scenario. Remove the invalid certificate fields from
testKubeconfigNoToken unless the test is intended to cover certificate-only
rejection; if so, replace them with valid certificate and private-key data.
Avoid broad HaveOccurred assertions that could accept unrelated parsing or
context-resolution failures.
In `@cli/kubeconfig.go`:
- Around line 74-79: Update the TLS configuration in LoadKubeConfig so
InsecureSkipVerify incorporates the kubeconfig-derived restConfig.Insecure or
TLSClientConfig.InsecureSkipTLSVerify value, rather than using only the
insecureSkipTLS CLI flag. Preserve the intended CLI-versus-kubeconfig precedence
and update the relevant tests to establish that behavior.
- Line 35: Update the kubeconfig loading setup around loadingRules to use
clientcmd.NewDefaultClientConfigLoadingRules() when kubeconfigPath is empty,
preserving default and KUBECONFIG-based precedence; set ExplicitPath only when a
non-empty override is supplied. Add a regression test covering an empty path
with KUBECONFIG set and verify the configured kubeconfig is loaded.
---
Nitpick comments:
In `@cli/kubeconfig.go`:
- Around line 31-34: Update the applicable AGENTS.md guidance for the
LoadKubeConfig CLI boundary if these rules are not already documented: specify
kubeconfig defaults, token-only authentication, TLS versus CA certificate
precedence, and the required test workflow. Keep the guidance scoped to this
package’s conventions and current behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ed461c5b-4e56-4ebb-9495-24d4b7a0c3aa
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (8)
cli/kubeconfig.gocli/kubeconfig_test.gocli/root.gocli/root_test.gocli/suite_test.gocli/version.gocli/version_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (5)
- cli/root_test.go
- cli/version.go
- cli/root.go
- cli/version_test.go
- go.mod
- Use NewDefaultClientConfigLoadingRules() to fall back to $KUBECONFIG/~/.kube/config - Honor kubeconfig insecure-skip-tls-verify cluster setting (OR with CLI flag) - Use specific error constant assertions in negative-path tests - Add tests for KUBECONFIG env fallback and kubeconfig insecure propagation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@xiormeesh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Adds the foundation for the
oc-olskubectl/oc plugin — a CLI for querying OpenShift Lightspeed from the terminal. This is the first PR in the OLS-1062 epic (9 stories total).Structure follows the oc-agentic CLI pattern: thin entry point in
cmd/oc-ols/, all command logic incli/. Unlike oc-agentic (which is a K8s API client via controller-runtime), oc-ols is a REST client that extracts bearer tokens and TLS config from kubeconfig to make HTTP calls to the OLS service endpoint.New direct dependencies:
spf13/cobra(CLI framework) andk8s.io/cli-runtime(IOStreams). Both are standard kubectl ecosystem libraries.oc-olsrequires token-based authentication (bearer token from kubeconfig). Client-certificate-only contexts (e.g. kubeadmin) are rejected — users mustoc loginwith username/password or SSO first. This is by design: OLS performs user-level authorization via the token, per.ai/spec/how/cli.md(Kubeconfig integration).Build:
go build -o /tmp/oc-ols ./cmd/oc-ols/Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
make test— full operator suite passes (0 failures, our code adds none)go build -o /tmp/oc-ols ./cmd/oc-ols/compiles successfully/tmp/oc-ols version→ "oc-ols dev"/tmp/oc-ols "hello"→ default mode dispatch stub on stderr/tmp/oc-ols --help→ shows global flags and version subcommandgo build -ldflags "-X github.com/openshift/lightspeed-operator/cli.Version=v0.1.0" -o /tmp/oc-ols ./cmd/oc-ols/ && /tmp/oc-ols version→ "oc-ols v0.1.0"Summary by CodeRabbit
New Features
oc-olscommand-line plugin entry point.Tests
Documentation